Make some of the node-level communication logging more clear to readers #12719
Conversation
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <BootstrapSdkVersion>10.0.100-rc.1.25451.107</BootstrapSdkVersion> | ||
| <BootstrapSdkVersion>10.0.100-rc.2.25466.101</BootstrapSdkVersion> |
There was a problem hiding this comment.
strictly speaking this is not necessary
| // Calculate salt from environment and tools directory | ||
| bool isNetTaskHost = IsHandshakeOptionEnabled(nodeType, NetTaskHostFlags); | ||
| string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT") ?? ""; | ||
| string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); |
There was a problem hiding this comment.
coalescing to empty string here makes it more annoying to do a clearer message string later, so let's leave it null for just a couple more lines.
There was a problem hiding this comment.
Pull Request Overview
This PR improves logging and tracing for command-line parameters and handshake options in MSBuild, and updates the bootstrap SDK version. The changes enhance debugging visibility by making trace output more informative and readable.
- Improves handshake options logging to show individual flag names instead of combined integer values
- Enhances command-line parameter tracing and environment variable handling with better null handling
- Updates bootstrap SDK version to rc.2
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Shared/CommunicationsUtilities.cs | Adds method to enumerate individual handshake options for better logging, improves trace output formatting, and enhances null handling for environment variables |
| src/MSBuild/XMake.cs | Updates command-line parameter tracing to properly format the string output |
| eng/Versions.props | Updates BootstrapSdkVersion from rc.1 to rc.2 |
| GatherAllSwitches(commandLine, out var switchesFromAutoResponseFile, out var switchesNotFromAutoResponseFile, out _); | ||
|
|
||
| CommunicationsUtilities.Trace($"Command line parameters: {commandLine}"); | ||
| CommunicationsUtilities.Trace($"Command line parameters: \"{string.Join(' ', commandLine)}\""); |
There was a problem hiding this comment.
The commandLine parameter is a string, not a collection. Using string.Join(' ', commandLine) will join individual characters with spaces instead of displaying the command line. Remove the string.Join call and use commandLine directly: CommunicationsUtilities.Trace($\"Command line parameters: \\\"{commandLine}\\\"\");
| CommunicationsUtilities.Trace($"Command line parameters: \"{string.Join(' ', commandLine)}\""); | |
| CommunicationsUtilities.Trace($"Command line parameters: \"{commandLine}\""); |
|
Can you show the text of your bad logs? I am a bit surprised that they got so mangled--maybe #12693 fixed it? |
|
You bet - just updated the description with a snippet showing the 'before' and 'after' scenarios. |
|
You're right - that other PR obviated most of this. |
Context
Some of the node commmunication data is hard to unpack, e.g. arrays are stringified as
System.String[]instead of useful content.Here's a sample:
On this sample log,
Changes Made
This small change correctly renders the command lines args for the node that's tracking communication, as well as the handshake options for easier reading.
Here's the 'after':
Testing
Notes